-
Notifications
You must be signed in to change notification settings - Fork 1.4k
nRF Desktop: nRF54LM20: execute the app code from RAM #25661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nRF Desktop: nRF54LM20: execute the app code from RAM #25661
Conversation
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 19ba58a47e03f8f3f8a52edf37c6d0bbe400b926 more detailssdk-nrf:
Github labels
List of changed files detected by CI (28)Outputs:ToolchainVersion: 4e36fbc961 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
25781ba to
8daea40
Compare
|
You can find the documentation preview for this PR here. Preview links for modified nRF Connect SDK documents: https://ncsdoc.z6.web.core.windows.net/PR-25661/nrf/applications/nrf_desktop/doc/dfu.html |
|
|
||
| &cpuapp_rram { | ||
| /* Application does not use cpuflpr core. Assign whole RRAM to cpuapp. */ | ||
| reg = < 0x0 DT_SIZE_K(2036) >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value value> style, no space after < and before >
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/images/mcuboot/app.overlay
Outdated
Show resolved
Hide resolved
8daea40 to
729480a
Compare
|
Pushed an update with the following additions:
|
729480a to
e4d08c0
Compare
| * RAM due to the dependency on the nrfutil device tool and its KMU | ||
| * provisioning functionality. | ||
| */ | ||
| nrf_kmu_reserved_push_area: memory@0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reserved memory is for hardware only, so KMU usage is fine, DMA usage is fine...
| * both the MCUboot image and the application code. Currently, it is used | ||
| * to share the image metadata between the bootloader and the application. | ||
| */ | ||
| cpuapp_sram_retained_mem_region: memory@400 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this though is not, this is normal RAM, it needs to be a child of the sram node, see the retention doc in zephyr for an example
| #address-cells = <1>; | ||
| #size-cells = <1>; | ||
| ranges = <0x0 0x7a400 DT_SIZE_K(20)>; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it can be moved here
| SYMBOLS __start_config_channel_modules __stop_config_channel_modules | ||
| KEEP SORT NAME) | ||
| if(CONFIG_DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_RAM_LOAD) | ||
| zephyr_linker_sources(RODATA linker/config_channel_dfu_ram_load.ld) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add zephyr_linker_section/zephyr_iterable_section too? This commit suggests that it's needed for IAR: b937d94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have to, as we just perform validation as part of this linker section without creating any new symbols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please check if the assertion is properly triggered for LLVM? (to ensure no silent errors in the future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed any more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... assuming that noone would try to compile the configuration with LLVM
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map.dtsi
Outdated
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map.dtsi
Outdated
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map_ram_load.dtsi
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/sysbuild.conf
Outdated
Show resolved
Hide resolved
b0749a4 to
f7436ee
Compare
alstrzebonski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - one comment to discuss tomorrow.
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/app_common.dtsi
Outdated
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map.dtsi
Outdated
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map_ram_load.dtsi
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/prj.conf
Outdated
Show resolved
Hide resolved
f7436ee to
6336962
Compare
|
Force pushed the PR to address review comments. |
6336962 to
f9806b6
Compare
| /* Validate that the executable RAM region does not overlap with the MCUboot RAM region. | ||
| * The validation process assumes a specific RAM layout in DTS - you can refer to the following | ||
| * file as an example: | ||
| * ./configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map_ram_load.dtsi | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can explain in comment why do we need to check that - mention that otherwise mcuboot would override its own RAM while copying app image to RAM.
| If the :kconfig:option:`CONFIG_DESKTOP_DFU_MCUMGR_MCUBOOT_DIRECT_XIP` option is enabled, the MCUmgr DFU module assumes that the bootloader simply boots the image with a higher version and does not confirm the newly updated image after a successful boot. | ||
| Make sure that :kconfig:option:`CONFIG_DESKTOP_DFU_MCUMGR_MCUBOOT_DIRECT_XIP` Kconfig option is enabled, if devices use the MCUboot bootloader in direct-xip mode without revert. | ||
| The option is enabled by default if :kconfig:option:`CONFIG_MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP` is enabled. | ||
| If the :kconfig:option:`CONFIG_MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP` option is enabled, the MCUmgr DFU module does not confirm the newly updated image after a successful boot as the confirmation is not supported in this bootloader mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| If the :kconfig:option:`CONFIG_MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP` option is enabled, the MCUmgr DFU module does not confirm the newly updated image after a successful boot as the confirmation is not supported in this bootloader mode. | |
| If the :kconfig:option:`CONFIG_MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP` option is enabled, the MCUmgr DFU module does not confirm the newly updated image after a successful boot, as the confirmation is not supported in this bootloader mode. |
| @@ -47,6 +47,61 @@ if(CONFIG_IMG_MANAGER) | |||
| zephyr_library_link_libraries(MCUBOOT_BOOTUTIL) | |||
| endif() | |||
|
|
|||
| if(CONFIG_BOOTLOADER_MCUBOOT) | |||
| if(NOT (CONFIG_MCUBOOT_BOOTLOADER_MODE_SINGLE_APP OR | |||
| CONFIG_MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP OR | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mcuboot_qspi configuration of nrf52840dk uses MCUboot with swap (CONFIG_MCUBOOT_BOOTLOADER_MODE_SWAP_USING_MOVE). It triggers the warning here (false positive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw. What do you think about enabling it explicitly to prevent breaking backwards compatibility if the default changes in scope of NCS?
Updated the Kconfig options of the DFU MCUmgr application module that is part of the nRF Desktop application. This module cannot be enabled when the MCUboot RAM load mode is used. The CONFIG_DESKTOP_DFU_MCUMGR_MCUBOOT_DIRECT_XIP Kconfig option was also removed to simplify maintainence. The module uses the CONFIG_MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP Kconfig option to identify the direct-xip mode. This option is directly supplied by the MCUboot component. Signed-off-by: Kamil Piszczek <[email protected]>
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map_ram_load.dtsi
Outdated
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map_ram_load.dtsi
Outdated
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map_ram_load.dtsi
Outdated
Show resolved
Hide resolved
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/prj_ram_load.conf
Show resolved
Hide resolved
|
|
||
| CONFIG_HEAP_MEM_POOL_SIZE=2560 | ||
|
|
||
| # Standard malloc implementation from the common libc library is not needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up action, we could disable it for all of the nRF Desktop configurations (to reduce memory usage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I open a follow-up ticket for this one?
| SB_CONFIG_MCUBOOT_MODE_RAM_LOAD=y | ||
| SB_CONFIG_BOOT_SIGNATURE_TYPE_ED25519=y | ||
| SB_CONFIG_BOOT_SIGNATURE_TYPE_PURE=y | ||
| SB_CONFIG_BOOT_SIGNATURE_KEY_FILE="\${APPLICATION_CONFIG_DIR}/images/mcuboot/mcuboot_private.pem" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider using a separate MCUboot private key as the configurations running from RAM are not interoperable with other configurations (similarly as we did for Fast Pair configurations)
dd97274 to
90a4ee3
Compare
90a4ee3 to
114de61
Compare
The reviewer is unavailable throughout this week
114de61 to
c653fcc
Compare
| * bootloader, the user must define the MCUBOOT_PRIMARY_SLOT_ID and MCUBOOT_SECONDARY_SLOT_ID | ||
| * macros. | ||
| */ | ||
| #define BUILD_TIME_DFU_SLOT_ID_INFO_IS_AVAILABLE \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw. We could consider skipping defining BUILD_TIME_DFU_SLOT_ID_INFO_IS_AVAILABLE and simply rely on existence of DFU_SLOT_ID macro: use DFU_SLOT_ID if defined, otherwise rely on dynamic APIs
| if(CONFIG_PARTITION_MANAGER_ENABLED OR (NOT CONFIG_USE_DT_CODE_PARTITION)) | ||
| message(FATAL_ERROR | ||
| "The RAM load mode of the MCUboot bootloader is not supported with the Partition " | ||
| "Manager (PM). Please disable PM and use DTS for defining the memory layout.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider mentioning about enabling CONFIG_USE_DT_CODE_PARTITION. Otherwise the displayed message might be a bit misleading in case CONFIG_PARTITION_MANAGER_ENABLED=n + CONFIG_USE_DT_CODE_PARTITION=n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use FATAL_ERROR here, especially not when asking user to disable partition manager, cause a fatal error here will prevent users from running menuconfig and correct the issue, and are instead forced to do a pristine build.
something which might not be desirable if the have done other customizations already in menuconfig.
If wanting a fatal error, then please turn it into a build time failure, not CMake configure time error.
See also zephyrproject-rtos/zephyr#9573
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| dt_reg_addr(mcuboot_ram_region_addr PATH ${mcuboot_ram_region_node}) | ||
| if(mcuboot_ram_region_addr LESS_EQUAL app_rxm_region_addr) | ||
| message(FATAL_ERROR | ||
| "The start address of the RXM region (0x${app_rxm_region_addr}) must be located " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider mentioning that it's an app-specific assumption (not a generic one). Maybe something similar to:
The validation process assumes a specific RAM layout in DTS - you can refer to the following file as an example: ./configuration/nrf54lm20dk_nrf54lm20a_cpuapp/memory_map_ram_load.dtsi
|
I am just waiting on the build system code owner review before this PR can be finalized |
tejlmand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid FATAL_ERRORs in CMake related to Kconfig settings.
| if(CONFIG_PARTITION_MANAGER_ENABLED OR (NOT CONFIG_USE_DT_CODE_PARTITION)) | ||
| message(FATAL_ERROR | ||
| "The RAM load mode of the MCUboot bootloader is not supported with the Partition " | ||
| "Manager (PM). Please disable PM and use DTS for defining the memory layout.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use FATAL_ERROR here, especially not when asking user to disable partition manager, cause a fatal error here will prevent users from running menuconfig and correct the issue, and are instead forced to do a pristine build.
something which might not be desirable if the have done other customizations already in menuconfig.
If wanting a fatal error, then please turn it into a build time failure, not CMake configure time error.
See also zephyrproject-rtos/zephyr#9573
| depends on $(dt_nodelabel_exists,cpuapp_sram_mcuboot_ram_region) | ||
| default $(dt_nodelabel_reg_addr_hex,cpuapp_sram_mcuboot_ram_region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we start to have several devicetree labels trying to achieve similar thing, which is to have the code executed / placed at a specific location.
Until we have some more common way to achieve this on higher level, then I would like to know why the existing CONFIG_SRAM_OFFSET is not suitable in this particular case for the MCUboot build ?
@nordicjm please have a look at this, and do some thinking regarding devicetree node used for features like this one, as well as the multicore TCM etc.
Other places of interest: zephyrproject-rtos/zephyr#91591 github.com//pull/25737
Not blocking comment for this PR, due to closeness to rc, but be prepared for follow-up work in this area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Kconfig option is only used in the context of the following linker script:
applications/nrf_desktop/linker/mcuboot_ram_load.ld
If set correctly, the CONFIG_SRAM_OFFSET would work in the context of the MCUboot image. However, in the MCUboot image context, we do not have access to the __rom_region_end symbol of the application image.
Also, you cannot change the CONFIG_SRAM_OFFSET Kconfig option from the default non-zero value using the standard *.conf file, as this is a promptless option. It is difficult to set this option correctly for the MCUboot at the nRF Desktop project level (you would have to force it via sysbuild)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra details.
Will be good to keep in mind when trying to find a better, more consistent / uniform solution for a recurrent problem on more advanced SoCs.
Added support for the RAM load mode of the MCUboot bootloader to the nRF Desktop application DFU module. Ref: NCSDK-35506 Signed-off-by: Kamil Piszczek <[email protected]>
Added the DTS RAM layout constraints to the nRF Desktop application when it uses the MCUboot bootloader in the RAM load mode. These constraints enforce a specific RAM layout for supported board targets. The user is required to define two DTS nodes that are part of the chosen SRAM DTS node of the application image: - cpuapp_sram_app_rxm_region - cpuapp_sram_mcuboot_ram_region Additionally, the cpuapp_sram_app_rxm_region DTS node must be placed before the cpuapp_sram_mcuboot_ram_region DTS node in the target device memory. Added a linker script with the assert statement that prevents the executable RAM region of the application image to overflow the standard RAM region of the MCUboot image. Ref: NCSDK-35506 Signed-off-by: Kamil Piszczek <[email protected]>
Added two nRF54LM20 DK configurations to the nRF Desktop application that improve the HID report rate performance over USB by executing application code from RAM. Due to this change, the application can now consistently maintain the 8000Hz report rate in the "release_ram_load" configuration variant. The second configuration variant called "ram_load" is used for debugging purposes. To execute code from RAM, the MCUboot has been configured for the RAM load mode. In this mode, the bootloader copies the newer application image from one of two RRAM slots into the RAM region and then it starts the application from RAM. To support RAM load mode of the MCUboot bootloader, devicetree (DTS) is used as a partitioning method. Currently, the default partitioning method for the nRF54LM20DK via the Partition Manager (PM) is not properly supported in the RAM load mode. Ref: NCSDK-35506 Signed-off-by: Aleksander Strzebonski <[email protected]> Signed-off-by: Kamil Piszczek <[email protected]>
c653fcc to
19ba58a
Compare
Side PRs:
CONFIG_BOOT_IMAGE_EXECUTABLE_RAM_STARTandCONFIG_BOOT_IMAGE_EXECUTABLE_RAM_SIZEaccording to the DTS description:[nrf noup] boot: zephyr: support code-sram label to define exe region sdk-mcuboot#575
Other potential improvements: